Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

type(feat): Implemented PanelUI Classes and Tests #338

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Temidayo32
Copy link
Collaborator

@Temidayo32 Temidayo32 commented Jan 26, 2025

Because

  • This commit implements PanelUI functionality

This commit

  • Implemented PanelUI Class and Tests

Fixes #313

@Temidayo32
Copy link
Collaborator Author

Temidayo32 commented Jan 26, 2025

This is still WIP. Failure is because tests do not meet 95% coverage yet.

@Temidayo32 Temidayo32 changed the title Implemented PanelUI Classes and Tests type(feat): Implemented PanelUI Classes and Tests Feb 3, 2025
@Temidayo32 Temidayo32 requested a review from b4handjr February 3, 2025 22:05
@Temidayo32 Temidayo32 force-pushed the panelui branch 4 times, most recently from 731b778 to 3d6cc7a Compare February 4, 2025 18:19
@Temidayo32 Temidayo32 force-pushed the panelui branch 5 times, most recently from e548f64 to 8d8f563 Compare February 5, 2025 14:26
Comment on lines 173 to 175
def verify_links_in_awesome_bar(self, links: list, new_window: bool = False) -> list:
"""
Verifies that the provided links appear in the awesome bar.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that were not found in the awesome bar.
"""
if new_window:
self.open_new_window()
else:
self.open_new_tab()
self.switch_to_new_window_or_tab()
for link in links:
self.selenium.get(link)
return self.awesome_bar(links)

def verify_private_browsing_links_not_in_awesome_bar(self, links: list) -> list:
"""
Verifies that the provided links visited in private browing session do not appear in the awesome bar.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that appeared in the awesome bar during private browsing.
"""
initial_window_handle = self.selenium.current_window_handle
self.open_private_window()
self.switch_to_new_window_or_tab()

for link in links:
self.selenium.get(link)

self.selenium.switch_to.window(initial_window_handle)
return self.awesome_bar(links)

def verify_links_in_history(self, links: list, new_window: bool = False) -> list:
"""
Verifies that the provided links appear in the history.
Args:
links `list`: A list of links to be verified.

Returns:
list: A list of links that were not found in the history.
"""
if new_window:
self.open_new_window()
else:
self.open_new_tab()
self.switch_to_new_window_or_tab()
for link in links:
self.selenium.get(link)
self.open_panel_menu()
self.open_history_menu()
urls = []
for link in links:
if not self.is_present(link):
urls.append(link)
return urls

def verify_private_browsing_links_not_in_history(
self,
links: list,
) -> list:
"""
Verifies that the provided links visited in private browsing session do not appear in the history.
Args:
links (`list[str]`): List of URLs visited during private browsing session
history (`History`): An instance of the History class used to check if a link is present in the history.

Returns:
list: A list of links that were found in the history during the private browsing session.
"""
invalid_links = []
initial_window_handle = self.selenium.current_window_handle
self.open_private_window()
self.switch_to_new_window_or_tab()

for link in links:
self.selenium.get(link)

self.selenium.switch_to.window(initial_window_handle)
self.open_panel_menu()
self.open_history_menu()
for link in links:
if self.is_present(link):
invalid_links.append(link)
return invalid_links

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the logic behind these additions? These seem like actions that a user would do. Are you expecting a user to supply a list of URLs for these methods to verify? Why wouldn't they just do that themselves?

Copy link
Collaborator Author

@Temidayo32 Temidayo32 Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. The actions mirror what a user would do manually, but given that this is a test automation tool, I think its purpose is always to automate manual user actions. Because technically, this isn't user functionality - it's testing infrastructure. In this sense, I try to automate certain expected behaviors of Private Window browsing vs Normal Window browsing, history etc.

Copy link
Collaborator Author

@Temidayo32 Temidayo32 Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the user will provide a list of URLs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, we are just trying to programmatically represent a way for a user to interact with Firefox. We don't want to assume what combinations they will do, just provide them a way to do them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes good sense. I will move those methods to tests. So they serve as part of FoxPuppet test suite. Will that be good or I just remove them totally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heading in the right direction with this. I think there will need to be some sort of History class so that we can contain the History specific interactions in one place.

@@ -129,6 +143,36 @@ def wait_for_bookmark(self) -> Bookmark:
)
return self.bookmark

def wait_for_panel_ui(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would a user want to wait for the panel ui to show? Can you explain a scenario?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual correct docstring:
Wait for the specified PanelUI item to be displayed.
That was misleading. There is no reason to wait for panel_ui as it is visible once the browser is opened.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PanelUI item, here, referring to menu items in the Panel Ui dropdown. History, for instance.

selenium.get(url)
panel_ui.open_history_menu()
panel_ui.clear_history()
import time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put this import at the top even if you just use it here

@Temidayo32
Copy link
Collaborator Author

Heading in the right direction with this. I think there will need to be some sort of History class so that we can contain the History specific interactions in one place.

Yes, I tried to do that initially. But this was difficult using the same property(history) and wait_for_history method defined on the BrowserWindow class as we did with others, especially using the Panel-UI, which in the dom is a list and all items are visible. Suppose we decided to extend later on by adding more list items like PrivateWindow or Download. In that case, it will prove difficult to interact with a specific one since they will all be visible.

Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent progress. I see some tests here that aren't quite testing the functionality of FoxPuppet but more so the functionality of Firefox. Can you identify which tests those are?

Comment on lines 44 to 65
"""Check if the provided links are present in the url bar's suggestions."""
urls = []
with self.selenium.context(self.selenium.CONTEXT_CHROME):
for link in links:
url_bar = self.selenium.find_element(*NavBarLocators.INPUT_FIELD)
url_bar.clear()
url_bar.send_keys(link)

self.wait.until(
lambda _: self.selenium.find_elements(*NavBarLocators.SEARCH_RESULTS)
)

search_results = self.selenium.find_elements(
*NavBarLocators.SEARCH_RESULT_ITEMS
)

for result in search_results:
url_span = result.find_element(*NavBarLocators.SEARCH_RESULT_ITEM)
if url_span.text in link:
if len(url_span.text) != 0:
urls.append(link)
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a separate method. If you need to create a UrlBar class to house it, that is fine, but this url_bar should be a property that just returns a class.

@@ -43,6 +40,20 @@ def create(
panel_items.update(PANEL_ITEMS)
return panel_items.get(_id, PanelUI)(window, root)

@property
def is_barged(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see where you get this name from but would a user understand what barged means? Honestly, if I didn't see the get_attribute("barged") I wouldn't know what it meant.

@Temidayo32 Temidayo32 requested a review from b4handjr February 12, 2025 17:12
Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. I think we can remove some checks and functions but there are also a few methods that we need to make sure they're doing what they say they will do.

Comment on lines 16 to 38
def check_suggestions(self, links: list) -> list:
"""Check if the provided links are present in the URL bar's suggestions."""
urls = []
with self.selenium.context(self.selenium.CONTEXT_CHROME):
for link in links:
url_bar = self.selenium.find_element(*URLBarLocators.INPUT_FIELD)
url_bar.clear()
url_bar.send_keys(link)

self.wait.until(
lambda _: self.selenium.find_elements(*URLBarLocators.SEARCH_RESULTS)
)

search_results = self.selenium.find_elements(
*URLBarLocators.SEARCH_RESULT_ITEMS
)

for result in search_results:
url_span = result.find_element(*URLBarLocators.SEARCH_RESULT_ITEM)
if url_span.text in link and len(url_span.text) != 0:
urls.append(link)
break
return urls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's save this for now and just return the URLs that get shown in the url bar that are suggested.

Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I think we're almost there.

Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes! Just a couple things and ill have Ben C look at this too.

@Temidayo32 Temidayo32 force-pushed the panelui branch 3 times, most recently from 4e18ec3 to 19a0b38 Compare February 21, 2025 13:44
Comment on lines +2 to +3
addopts = -vvv --driver Firefox --cov --cov-fail-under=95 --html=results/report.html --self-contained-html
testpaths = tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline

Copy link
Contributor

@b4handjr b4handjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these based on our chat

""",
self.selenium.find_element(*PanelUILocators.HISTORY_DIALOG_BUTTON),
)
self.selenium.switch_to.default_content()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to switch_to since we are using with

) -> None:
"""Test that links opened in new window are present in browser history."""
panel_ui.open_new_window()
time.sleep(3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to get rid of this if we can.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could too, even the blank page has something you could wait for, iirc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you could even do this:

  • Get the len() of selenium.window_handles (call it old_num_windows or something)
  • open new window
  • using Wait(), do `wait.until(lambda _: len(selenium.window_handles) < old_num_windows)

Not entirely sure exactly this would work, but something like it probably will

Comment on lines +152 to +155
def clear_history(self):
"""
Clears the browsing history.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can confirm the history is cleared some way. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the history_items method

Copy link
Collaborator

@ben-c-at-moz ben-c-at-moz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good, I think addressing that sleep command is the only new thing I can find in here that I think needs to change. I would definitely make absolutely sure you don't run find_elements() on anything until you KNOW it exists, as it doesn't include the same implicit wait as find_element() does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add PanelUI functionality
3 participants